Conversation
|
This is breaking the implementation in Craft CMS's test class, as the Yii2 Module now is a "final" class? This is not really BC-compatible. If intended, it should atleast be a new version? |
|
You are right, changing classes to final should have been a major version update. Are you up for making a PR that reverts the final change? Note that we will create a new major release where these classes will be final. Extending 3rd party classes is generally not the way to go as your code becomes very strongly coupled to the library. |
|
Un-finalized the module in |
|
Tagged release. |
|
Tagging @brandonkelly, which has code that extends this module: https://github.com/craftcms/cms/blob/5.x/src/test/Craft.php |
I haven't tested it any further, will do tomorrow. |
|
@brandonkelly the latest release of this module results in the following: Types need to be updated accordingly in Craft as well |
|
Is the inheritance really needed? Why not use composition?
…On Tue, 10 Dec 2024, 08:04 Bob Olde Hampsink, ***@***.***> wrote:
@brandonkelly <https://github.com/brandonkelly> the latest release of
this module results in the following:
PHP Fatal error: Declaration of craft\test\CraftConnector::startApp(?yii\log\Logger $logger = null) must be compatible with Codeception\Lib\Connector\Yii2::startApp(?yii\log\Logger $logger = null): void in /app/user/vendor/craftcms/cms/src/test/CraftConnector.php on line 135
Types need to be updated accordingly in Craft as well
—
Reply to this email directly, view it on GitHub
<#96 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEFRTNSWOBB5EHZLHJFQXT2E2G7ZAVCNFSM6AAAAABTIENMWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMZQGYYDOMBUHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
This would be something for the Craft team to consider. To get some insight, here is how they are using this module: https://github.com/craftcms/cms/tree/5.x/src/test By the way, I think even changing typing like this is worth a new major release, as its still not backward compatible. |
|
@boboldehampsink We updated the return type for |
I agree, it was a mistake by me, which is why we reverted. Itll be in a major version bump. |
Thanks! Any chance of a backport for v4 as well? |
|
@boboldehampsink We didn’t override that method until Craft 5. And Craft 4 is stuck on codeception/module-yii2 1.1.5, as 1.1.6 is when they started requiring Codeception 5. If you’re using Codeception 5 with Craft 4, please PR whatever changes you need to Craft. Assuming they don’t break our own tests, we can pull them in. |
|
@brandonkelly we're using Codeception 5 with Craft 4 indeed. That would just be the same fix, correct? |
|
As I said, we weren’t overriding that method until Craft 5. |
This PR improves typing and fixes issues identified by phpstan.
It also removes some functions that were already marked as
@internal(so it doesn't count as breaking backward compatibility)